[WIP][SQL] Put check in ExpressionEncoder.fromRow to ensure we can convert deserialized object to required type #16546
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ialized object to required type.
|
Test build #71192 has started for PR 16546 at commit |
|
retest this please. |
|
Test build #71196 has finished for PR 16546 at commit
|
|
Just noticed that there is a related and interesting pr #16240 merged recently. |
|
Test build #71218 has finished for PR 16546 at commit
|
| if (!(t <:< localTypeOf[Seq[_]])) { | ||
| return None | ||
| } | ||
| val TypeRef(_, _, elementTypeList) = t |
There was a problem hiding this comment.
This would probably be better solved by using match and typeParams. Something like this:
t match {
case TypeRef(_, _, Seq(elementType)) => Some(elementType)
case _ =>
t.baseClasses.find { c =>
val cType = c.asClass.toType
cType <:< localTypeOf[Seq[_]] && cType.typeParams.nonEmpty
}.map(t.baseType(_).typeParams.head)
}Also not sure whether types with more than one type parameter are handled correctly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Two problems are addressed in this patch.
Seq[_]which doesn't have element typeCurrently, in
ScalaReflection.serializerFor, we try to serialize all sub types ofSeq[_]. But forRangewhich is aSeq[Int]and doesn't have element type,serializerForwill fail and show mystery messages:This patch tries to fix this by considering the types without element type.
We serialize the objects with common super class such as
Seq[_]to a common internal data. But when we want to deserialize the internal data back to the original objects, we will encounter the problem of initialization of different types of objects.For example, we deserialize the data serialized from
Seq[_]toWrappedArray. It works when we serialize data ofSeq[_]. If we try to serialize data of subclass ofSeq[_](for exampleRange) which is not assignable fromWrappedArray, there will be runtime error when converting deserialized data to the required subclass ofSeq[_].Except for explicitly writing down the rule to deserialize each subclass of
Seq[_], I think the feasible solution is to check if we can convert deserialized data to the required type. This patch puts the check intoExpressionEncoder.fromRow. Once the requirement is not matched, we show a reasonable message to users.How was this patch tested?
Jenkins tests.
Please review http://spark.apache.org/contributing.html before opening a pull request.